-
Notifications
You must be signed in to change notification settings - Fork 50
refactor(shred-network): consolidate threads #980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
the following threads are all eliminated and the logic is run by the shred receiver thread - two packet handler threads for turbine and repair - shred verifier thread
- unreachable: assume capacity when no capacity is guaranteed - double free: deinit items in a recycled list without clearing the list, then deinit again on the next usage of the list
Codecov Report❌ Patch coverage is
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
…nsolidate-threads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions, otherwise I'm a fan of this step forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement 👍
The current structure of the shred network is copied directly from agave. Each pair of
[square brackets]represents a thread. In agave they use different names, and some of the components are duplicated on multiple threads in agave where we use just one, but otherwise the architecture is the same. I simply copied agave without thinking about how to improve the design.This design is pointlessly complicated. It makes the code hard to understand and hard to debug. This is a sequence of steps that need to happen in order, and there's no good reason why they shouldn't happen in the same thread.
I've started to reduce the number of threads here by consolidating the logic from the shred receiver, verifier, and processor into a single thread, and consolidate the packet tagging into the socket threads. Here's the new approach I have in the current PR:
I'd like to also flatten down the socket threads so that this entire thing becomes a single thread, but that's going to require a more complicated rework of our networking code.
I haven't done much benchmarking yet, but the performance appears to be unchanged by this refactor. Before and after this change, sig is able to process about 40 slots worth of mainnet shreds per second on my computer running on my home network.
Future optimizations
If in the future, if the number of shreds skyrockets and we really need multiple threads to handle them, then it will require some more rework, but not a return to the old design. The old design was overly pipelined. If you need more parallelism, you really only need to define two single threaded tasks:
You can parallelize task 1 in a thread pool if necessary. Currently we have task one split into six different threads that all do different things and I don't see any reason to keep this up.